Xml attribute tidy up #4971

jmarshallnz · 2014-07-01T02:08:36Z

This includes fix for possible crashes (null ptr deref) which should be backported to Gotham, plus additional fixes for not reading Attribute's into CStdString (which has null ptr protection) so that when we replace with std::string we don't introduce a bunch of further null ptr deref's.

It mostly touches various settings:

Addon settings. Currently the 'type' attribute has been read case-sensitive in some spots, and case-insensitive in others. I've changed to case-sensitive everywhere.
Addon settings. A few other attributes are already case-sensitive (e.g. options) so I've changed a couple others where I found them. In particular, subsetting='true', the true is case-sensitive, and where $HOURS is used, that is now case-sensitive as well.
The bydate attribute of AdvancedSettings tvshow matching regexp is now case-sensitive.
Skin settings. We write these to guisettings.xml, so I've made the type case-sensitive.

Otherwise, just general cleanup.

This needs to go in before the next CStdString removal, so sooner the better on reviews please!

…ion of an attribute

jmarshallnz · 2014-07-01T02:08:47Z

jenkins build this please

jmarshallnz · 2014-07-01T10:05:03Z

jenkins build this please

Montellese · 2014-07-01T13:17:18Z

Couldn't find anything wrong. Nice cleanup/refactor.

xbmc/addons/GUIDialogAddonSettings.cpp

-      bSort=true;
-
-    if (type)
+    const std::string type = XMLUtils::GetAttribute(setting, "type");


…ectly to CStdString/std::string, as they may be NULL

…pared case-sensitive

… made case-sensitive

…sed enough to warrant the include)

…s things up

…-insensitive (we write them, we read them)

…ase-sensitive to drop some strcmpi()

…exps case-sensitive (i.e. 'true')

jmarshallnz · 2014-07-01T20:16:26Z

Thanks for the review! Updated as per comments.

jenkins build this please

Xml attribute tidy up

MilhouseVH · 2014-07-06T19:31:59Z

There is a problem with PR4971 - now that this is merged, is it better to open a new issue?

Assuming the discussion can continue here, the addon Artwork Downloader is missing several label and textual elements from the Settings dialog once this PR is added.

Before #1 - without PR4971 (fde760b):

After #1 - with PR4971 (a362942):

Before #2 - without PR4971 (fde760b):

After #2 - with PR4971 (a362942):

See also here.

Given the changes made by this PR, I suppose it's equally possible that these addons that are now missing textual elements may require updating to be case-sensitive (ping @MartijnKaijser).

jmarshallnz · 2014-07-06T21:42:57Z

Add-on settings were already case-sensitive in bits, but not case-sensitive in other bits, so yes, it needs fixing in the add-ons.

MilhouseVH · 2014-07-07T01:14:31Z

This isn't due to case-insensitivity, however.

Addon Setting controls are no longer being created in GUIDialogAddonSettings.cpp unless an id property is present, which means that sep and lsep controls are now ignored by this PR (as they have no id property).

The Artwork Downloader "Info" panel above that is no longer working can be viewed here. It looks correct, and should be displayed, but isn't.

Removing the null check on id fixes this problem.

jmarshallnz · 2014-07-07T07:30:47Z

Thanks for investigating - most useful. It's not quite as easy as removing the null check, as that's needed, but perhaps can be moved to where it's needed (we don't care about it for separator controls for example).

Will look into it.

Mafarricos · 2014-07-07T16:31:47Z

This problem also affects action controls, in trakt addon for example:

Screens from XBMC 13.1:
The force option:

The Movie and Service lsep:

And other lsep's:

Latest Helix Release for windows:
The force option is gone:

The Movie and Service is gone:

And other lsep's also gone:

What is needed to change in the addons so the lsep's, sep's and action controls appear after this PR?

MilhouseVH · 2014-07-07T16:35:49Z

Nothing needs to change in the addons, this PR needs updating so that it doesn't ignore controls with no id property.

Mafarricos · 2014-07-07T16:37:20Z

OK, I understood that something could be made in the setting so the controls would appear, but that way there's nothing to do in the addon matter. Thanks, will wait for PR update.

MilhouseVH · 2014-07-13T11:16:05Z

@Mafarricos - the sep/lsep issue is fixed by #5025 which is in build #0713 on the forum. If you find any problems please update #5025, thanks.

Mafarricos · 2014-07-14T09:01:47Z

@MilhouseVH it seems that all is fixed with this PR.
Thanks!

[xmlutils] adds helper GetAttribute for retrieving a std::string vers…

06626d6

…ion of an attribute

t-nelson reviewed Jul 1, 2014
View reviewed changes

xbmc/addons/GUIDialogAddonSettings.cpp

bSort=true;

if (type)

const std::string type = XMLUtils::GetAttribute(setting, "type");

This comment was marked as spam.

Sign in to view

Jonathan Marshall added 12 commits July 2, 2014 08:08

[xmlutils] use XMLUtils::GetAttribute() when assigning attributes dir…

4d16040

…ectly to CStdString/std::string, as they may be NULL

[xml] fix potential null pointer dereference when reading attributes

2ce4ccc

[addons] make the 'type' attribute of addon settings consistently com…

2eecefd

…pared case-sensitive

[addons] read the id attribute into a std::string where comparison is…

fee8d45

… made case-sensitive

[xml] use GetAttribute() where it makes the code clearer (and where u…

ca295a1

…sed enough to warrant the include)

[xml] use TiXmlNode::Attribute(foo, int|double) method where it tidie…

29e767b

…s things up

[xml] read attributes into const char* where they're re-used immediately

a42ee0d

[skinsettings] setting type of skin settings needn't be compared case…

4b78439

…-insensitive (we write them, we read them)

[addons] have the 'subsetting' attribute and '' values attribute be c…

6f5cd02

…ase-sensitive to drop some strcmpi()

[settings] make the advancedsetting 'bydate' attribute for tvshow reg…

d2a666d

…exps case-sensitive (i.e. 'true')

[addons] drop duplicate if clause

4b6c3b9

[cosmetic] align assignment block

a362942

jmarshallnz added a commit that referenced this pull request Jul 1, 2014

Merge pull request #4971 from jmarshallnz/xml_attribute

1406843

Xml attribute tidy up

jmarshallnz merged commit 1406843 into xbmc:master Jul 1, 2014

jmarshallnz deleted the xml_attribute branch July 1, 2014 22:12

MartijnKaijser added this to the Helix 14.0-alpha1 milestone Jul 16, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xml attribute tidy up #4971

Xml attribute tidy up #4971

jmarshallnz commented Jul 1, 2014

jmarshallnz commented Jul 1, 2014

jmarshallnz commented Jul 1, 2014

Montellese commented Jul 1, 2014

This comment was marked as spam.

jmarshallnz commented Jul 1, 2014

MilhouseVH commented Jul 6, 2014

jmarshallnz commented Jul 6, 2014

MilhouseVH commented Jul 7, 2014

jmarshallnz commented Jul 7, 2014

Mafarricos commented Jul 7, 2014

MilhouseVH commented Jul 7, 2014

Mafarricos commented Jul 7, 2014

MilhouseVH commented Jul 13, 2014

Mafarricos commented Jul 14, 2014

Xml attribute tidy up #4971

Xml attribute tidy up #4971

Conversation

jmarshallnz commented Jul 1, 2014

jmarshallnz commented Jul 1, 2014

jmarshallnz commented Jul 1, 2014

Montellese commented Jul 1, 2014

This comment was marked as spam.

jmarshallnz commented Jul 1, 2014

MilhouseVH commented Jul 6, 2014

jmarshallnz commented Jul 6, 2014

MilhouseVH commented Jul 7, 2014

jmarshallnz commented Jul 7, 2014

Mafarricos commented Jul 7, 2014

MilhouseVH commented Jul 7, 2014

Mafarricos commented Jul 7, 2014

MilhouseVH commented Jul 13, 2014

Mafarricos commented Jul 14, 2014